Add API to set CMake generator default (eg Ninja) #1046
Conversation
ISV can set their desired CMake generator default (eg Ninja). Addresses Issue: CDT CMake Improvements eclipse-cdt#1000, IDE-82683-REQ-012 CMake generator default
|
Hi @jonahgraham , WRT our discussion on the yaml file yesterday, as far as I can tell the org.eclipse.cdt.cmake.core.internal.CMakePropertiesController.save(ICMakeProperties) method is never called by CDT code, which means the yaml file will not exist for regular (non-ISV extended) CDT products. You'll see that I've added an API for setting the cmake generator. |
Test Results 601 files - 34 601 suites - 34 12m 56s ⏱️ - 42m 47s For more details on these errors, see this check. Results for commit c8d0797. ± Comparison against base commit 1abb990. This pull request removes 1217 tests. |
There was a problem hiding this comment.
WRT our discussion on the yaml file yesterday, as far as I can tell the org.eclipse.cdt.cmake.core.internal.CMakePropertiesController.save(ICMakeProperties) method is never called by CDT code, which means the yaml file will not exist for regular (non-ISV extended) CDT products.
Let me know if you think I should still consider the yaml file questions for an ISV modified CDT product.
I don't think you need to consider the yaml file - thank you for pointing out that save was never called. I guess this was incomplete work once upon a time.
I guess that you will start calling save now though (in a subsequent commit)? Perhaps after migrating from yaml to something else?
| overrides = cmakeProperties.getLinuxOverrides(); | ||
| } | ||
| return getUiOrOsOverrides(overrides); | ||
| return getUiOrOsOverrides(getOsOverrides(cmakeProperties)); |
There was a problem hiding this comment.
Unfortunately this change gave me a stack overflow. I created a new CMake project and hit build (in the launch bar) and I got:
!ENTRY org.eclipse.core.jobs 4 2 2025-01-19 10:41:21.640
!MESSAGE An internal error occurred during: "Building Active Configuration".
!STACK 0
java.lang.StackOverflowError
at org.eclipse.cdt.cmake.core.CMakeBuildConfiguration$SimpleOsOverridesSelector.getOsOverrides(CMakeBuildConfiguration.java:637)
at org.eclipse.cdt.cmake.core.CMakeBuildConfiguration$SimpleOsOverridesSelector.getOsOverrides(CMakeBuildConfiguration.java:637)
at org.eclipse.cdt.cmake.core.CMakeBuildConfiguration$SimpleOsOverridesSelector.getOsOverrides(CMakeBuildConfiguration.java:637)
-- repeating --There was a problem hiding this comment.
Apologies, that was some clumsy refactoring last thing on a Friday when I needed to finish work.
| linuxOverrides.setDefaultGenerator(CMakeGenerator.UnixMakefiles); | ||
| // reset(true) causes the CMake generator to be reset to it's default value. | ||
| properties.reset(true); | ||
| setDefaultCMakeProperties(properties); |
There was a problem hiding this comment.
This isn't ok - you shouldn't have non-final methods called by constructor.
Can you change this around so that the interface has getDefaultCMakeProperties that extenders can override if they don't like the built-in defaults?
Here is a diff showing what I mean, but it isn't tested, so may require fixing up
expand for patch contents
diff --git a/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/CMakeBuildConfiguration.java b/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/CMakeBuildConfiguration.java
index fe7f365c83d..a0eeee42967 100644
--- a/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/CMakeBuildConfiguration.java
+++ b/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/CMakeBuildConfiguration.java
@@ -77,7 +77,6 @@ import org.osgi.service.prefs.Preferences;
*/
public class CMakeBuildConfiguration extends CBuildConfiguration implements ICMakeBuildConfiguration {
- private static final String CMAKE_PROPERTIES_INITIALSED = "cmake.properties.initialised"; //$NON-NLS-1$
public static final String CMAKE_USE_UI_OVERRIDES = "cmake.use.ui.overrides"; //$NON-NLS-1$
public static final boolean CMAKE_USE_UI_OVERRIDES_DEFAULT = false;
public static final String CMAKE_GENERATOR = "cmake.generator"; //$NON-NLS-1$
@@ -93,8 +92,6 @@ public class CMakeBuildConfiguration extends CBuildConfiguration implements ICMa
// lazily instantiated..
private ICMakePropertiesController pc;
- ICMakeProperties defaultProperties;
-
private Map<IResource, IScannerInfo> infoPerResource;
/**
* whether one of the CMakeLists.txt files in the project has been modified and saved by the
@@ -116,7 +113,6 @@ public class CMakeBuildConfiguration extends CBuildConfiguration implements ICMa
ICMakeToolChainManager manager = Activator.getService(ICMakeToolChainManager.class);
toolChainFile = manager.getToolChainFileFor(getToolChain());
- setDefaultCMakeProperties();
}
public CMakeBuildConfiguration(IBuildConfiguration config, String name, IToolChain toolChain) {
@@ -127,30 +123,21 @@ public class CMakeBuildConfiguration extends CBuildConfiguration implements ICMa
ICMakeToolChainFile toolChainFile, String launchMode) {
super(config, name, toolChain, launchMode);
this.toolChainFile = toolChainFile;
- // // moule
- setDefaultCMakeProperties();
- }
-
- private void setDefaultCMakeProperties() {
- // Only set defaults on initial construction
- if (getProperty(CMAKE_PROPERTIES_INITIALSED) == null) {
- ICMakeProperties properties = CMakePropertiesFactory.getProperties();
- IOsOverrides windowsOverrides = properties.getWindowsOverrides();
- windowsOverrides.setDefaultGenerator(CMakeGenerator.UnixMakefiles);
-
- IOsOverrides linuxOverrides = properties.getLinuxOverrides();
- linuxOverrides.setDefaultGenerator(CMakeGenerator.UnixMakefiles);
- // reset(true) causes the CMake generator to be reset to it's default value.
- properties.reset(true);
- setDefaultCMakeProperties(properties);
- }
}
@Override
- public void setDefaultCMakeProperties(ICMakeProperties defaultProperties) {
- this.defaultProperties = defaultProperties;
- setProperty(CMAKE_GENERATOR, getOsOverrides(defaultProperties).getGenerator().getCMakeName());
- setProperty(CMAKE_PROPERTIES_INITIALSED, Boolean.TRUE.toString());
+ public ICMakeProperties getDefaultCMakeProperties() {
+ ICMakeProperties properties = CMakePropertiesFactory.createProperties();
+ IOsOverrides windowsOverrides = properties.getWindowsOverrides();
+ windowsOverrides.setDefaultGenerator(CMakeGenerator.UnixMakefiles);
+
+ IOsOverrides linuxOverrides = properties.getLinuxOverrides();
+ linuxOverrides.setDefaultGenerator(CMakeGenerator.UnixMakefiles);
+ // reset(true) causes the CMake generator to be reset to it's default value.
+ properties.reset(true);
+
+ setProperty(CMAKE_GENERATOR, getOsOverrides(properties).getGenerator().getCMakeName());
+ return properties;
}
/**
@@ -192,7 +179,7 @@ public class CMakeBuildConfiguration extends CBuildConfiguration implements ICMa
}
// ICMakeProperties cmakeProperties = getPropertiesController().load();
- ICMakeProperties cmakeProperties = getPropertiesController().load(defaultProperties);
+ ICMakeProperties cmakeProperties = getPropertiesController().load(getDefaultCMakeProperties());
runCMake |= !Files.exists(buildDir.resolve("CMakeCache.txt")); //$NON-NLS-1$
// Causes CMAKE_BUILD_TYPE to be set according to the launch mode
@@ -334,7 +321,7 @@ public class CMakeBuildConfiguration extends CBuildConfiguration implements ICMa
project.deleteMarkers(ICModelMarker.C_MODEL_PROBLEM_MARKER, false, IResource.DEPTH_INFINITE);
// ICMakeProperties cmakeProperties = getPropertiesController().load();
- ICMakeProperties cmakeProperties = getPropertiesController().load(defaultProperties);
+ ICMakeProperties cmakeProperties = getPropertiesController().load(getDefaultCMakeProperties());
CommandDescriptorBuilder cmdBuilder = new CommandDescriptorBuilder(cmakeProperties,
new SimpleOsOverridesSelector());
CommandDescriptor command = cmdBuilder.makeCMakeBuildCommandline(getCleanCommand());
@@ -436,7 +423,7 @@ public class CMakeBuildConfiguration extends CBuildConfiguration implements ICMa
@SuppressWarnings("unchecked")
public <T> T getAdapter(Class<T> adapter) {
if (ICMakePropertiesController.class.equals(adapter)) {
- // return (T) pc;
+ // return (T) pc;
return (T) getPropertiesController();
}
return super.getAdapter(adapter);
diff --git a/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/ICMakeBuildConfiguration.java b/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/ICMakeBuildConfiguration.java
index 5769704217e..913d3e369bc 100644
--- a/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/ICMakeBuildConfiguration.java
+++ b/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/ICMakeBuildConfiguration.java
@@ -1,3 +1,13 @@
+/*******************************************************************************
+ * Copyright (c) 2015, 2025 QNX Software Systems and others.
+ *
+ * This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License 2.0
+ * which accompanies this distribution, and is available at
+ * https://www.eclipse.org/legal/epl-2.0/
+ *
+ * SPDX-License-Identifier: EPL-2.0
+ *******************************************************************************/
package org.eclipse.cdt.cmake.core;
import org.eclipse.cdt.cmake.core.properties.ICMakeProperties;
@@ -14,5 +24,5 @@ public interface ICMakeBuildConfiguration {
* {@link CMakeBuildConfiguration#CMAKE_GENERATOR}. The CMake generator property can then be accessed using
* the {@link ICBuildConfiguration#getProperty(String)}.
*/
- void setDefaultCMakeProperties(ICMakeProperties defaultProperties);
+ ICMakeProperties getDefaultCMakeProperties();
}
diff --git a/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/properties/CMakePropertiesFactory.java b/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/properties/CMakePropertiesFactory.java
index 5d679ded86f..8c5c5302ceb 100644
--- a/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/properties/CMakePropertiesFactory.java
+++ b/cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/properties/CMakePropertiesFactory.java
@@ -1,9 +1,22 @@
+/*******************************************************************************
+ * Copyright (c) 2015, 2025 QNX Software Systems and others.
+ *
+ * This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License 2.0
+ * which accompanies this distribution, and is available at
+ * https://www.eclipse.org/legal/epl-2.0/
+ *
+ * SPDX-License-Identifier: EPL-2.0
+ *******************************************************************************/
package org.eclipse.cdt.cmake.core.properties;
import org.eclipse.cdt.cmake.core.internal.properties.CMakePropertiesBean;
+/**
+ * @since 2.0
+ */
public class CMakePropertiesFactory {
- public static ICMakeProperties getProperties() {
+ public static ICMakeProperties createProperties() {
return new CMakePropertiesBean();
}
}There was a problem hiding this comment.
Addendum,
extenders can do something like this:
@Override
public ICMakeProperties getDefaultCMakeProperties() {
// get the built-in CDT defaults
ICMakeProperties properties = super.getDefaultCMakeProperties();
// provide my customizations (I don't know what Renesas/extenders may actually want to do here)
properties.setWarnUnitialized(true);
return properties;
}There was a problem hiding this comment.
And finally - maybe ;) - I think calling:
setProperty(CMAKE_GENERATOR, getOsOverrides(properties).getGenerator().getCMakeName());
from within this method is incorrect as that method has side effects by changing project preferences too. It may be best to call setProperty on the return value of getDefaultCMakeProperties:
private ICMakeProperties getDefaultPropertiesInternal() {
ICMakeProperties properties = getDefaultCMakeProperties();
setProperty(CMAKE_GENERATOR, getOsOverrides(properties).getGenerator().getCMakeName());
return properties;
}and then in build/clean:
ICMakeProperties cmakeProperties = getPropertiesController().load(getDefaultPropertiesInternal());
There was a problem hiding this comment.
Hi @jonahgraham ,
thanks for taking the time to review this.
I like all your suggestions here and think the structure is better the way you have suggested.
However, it removes a key requirement. Each time the build is performed with this patch, it uses the default values. If the user changes the generator value in the UI, that change is ignored. Hence the need for the CMAKE_PROPERTIES_INITIALSED.
I'm struggling to come up with a better way of doing things at the moment, so if you have any ideas I'd be grateful of them please.
Maybe using CMakePropertiesController.save(ICMakeProperties) will help with the issue I mentioned in my previous comment about CMAKE_PROPERTIES_INITIALSED. But as well as saving the props, I'd need to do some other stuff in the load, like: setProperty(CMAKE_GENERATOR, getPlatformOsOverrides(properties).getGenerator().getCMakeName()); |
|
@betamaxbandit I think if the load/save are matched then the problem is resolved - right now it seems a bit mismatched between the incomplete yaml and the launch config properties. Can we get it down to only one place that user information is stored? |
ISV can set their desired CMake generator default (eg Ninja).
Addresses Issue: CDT CMake Improvements #1000, IDE-82683-REQ-012 CMake generator default